Consolidate offset - length behavior in files by rakshith91 · Pull Request #7942 · Azure/azure-sdk-for-python

rakshith91 · 2019-10-16T17:56:13Z

Resolves #7812

change offset - length behavior
fix a small issue in from_file_url

adxsdk6 · 2019-10-16T17:56:15Z

Can one of the admins verify this patch?

kristapratico · 2019-10-16T18:38:08Z

-            raise ValueError("end_range must be an integer that aligns with 512 file size")
-        content_range = "bytes={0}-{1}".format(start_range, end_range)
+        if offset is None or offset % 512 != 0:
+            raise ValueError("offset must be an integer that aligns with 512 file size")


kristapratico · 2019-10-16T18:41:09Z

-        source_range = 'bytes={0}-{1}'.format(source_range_start, source_range_start + (range_end - range_start))
+        end_range = offset + length - 1
+        destination_range = 'bytes={0}-{1}'.format(offset, end_range)
+        source_range = 'bytes={0}-{1}'.format(source_offset, source_offset + length - 1)  # should subtract 1 here?


remove comment

kristapratico · 2019-10-16T18:43:18Z

-            raise ValueError("end_range must be an integer that aligns with 512 file size")
-        content_range = 'bytes={0}-{1}'.format(start_range, end_range)
+        if offset is None or offset % 512 != 0:
+            raise ValueError("offset must be an integer that aligns with 512 file size")


xiafu-msft · 2019-10-17T05:03:45Z

-                content_range = "bytes={0}-{1}".format(start_range, end_range)
+        if offset is not None:
+            if length is not None:
+                length = offset + length - 1  # Reformat to an inclusive range index


Is there a reason we reuse length here? can we make it offset_end = offset + length - 1

This one hasn't been addressed 😀

Thanks @xiafu-msft - I'm merging these changes into the Stream PR now - will add this
FYI @rakshith91

xiafu-msft · 2019-10-17T05:14:29Z

            raise ValueError("Offset value must not be None if length is set.")
-
+        if length is not None:
+            length = offset + length - 1  # Service actually uses an end-range inclusive index


can we rename this length to offset_end or range_end?

xiafu-msft · 2019-10-17T05:20:00Z

-        content_range = 'bytes={0}-{1}'.format(start_range, end_range)
+        if offset is None or offset % 512 != 0:
+            raise ValueError("offset must be an integer that aligns with 512 file size")
+        if length is None or length % 512 != 511:


I think here it should be:
if length is None or length % 512 != 0:
or
end_range = length + offset - 1
if end_range is None or end_range % 512 != 511:
😀

xiafu-msft · 2019-10-17T06:11:12Z

-        content_range = "bytes={0}-{1}".format(start_range, end_range)
+        if offset is None or offset % 512 != 0:
+            raise ValueError("offset must be an integer that aligns with 512 file size")
+        if length is None or length % 512 != 511:


change 511 to 0?

rakshith91 · 2019-10-17T18:22:21Z

https://dev.azure.com/azure-sdk/public/_build/results?buildId=153931&view=results

ci is green

* Fix SubStream to respect IOBase protocol (#7843) * removes NoRetry policy (#7845) * [storage] Makes signed_identifiers a required param (#7844) * makes signed_identifiers a required param * fix tests and update history * removes unecessary check * fix files and queues tests * [storage] Changes `file_permission_key` param to `permission_key` (#7841) * changes file_permission_key param to permission_key * update history * Change the param directive to keyword in docstrings (#7855) * Change the directive to keyword in docstrings * pylint :( * [storage] Adds from_string to models (#7870) * adds from_string to models * fixes to naming and history * remove _str in docstrings * pylint * [storage-file] await async poller (#7872) * await files async poller * fix docstrings * fix type hints * [File and Queue] Client constructors (#7853) * from_queue_url * file client constructors * update CHANGELOG * fix test * lint fix * some doc changes * queue docs * blob docs * [storage-queue] rename queue messages (#7895) * rename queue messages * update history * [Storage-Queue][Storage-File] Kwargify positional params (#7877) * Queue kwargification * kwargify Files * small fix * minor fix * comments + lint * some changes * pylint * [storage-file, queue] moves param protocol to kwargs in gen_shared_access_signature() (#7897) * moves protocol in gen_sas to kwargs * delete whitespace * [storage] Unexposes models from aio (#7881) * unexposes models from aio * unexpose all except clients and pylint fixes * update history * unexpose sync paged models * update history phrasing * [Storage-queue] Allow None message encode policy (#7898) * Don't expose NoEncode policies * Support None value for encode/decode policies * Updated tests * Updated sync encryption tests * Updated async encryption tests * Updated async encoding tests * Fixed test * Fix test * Updated release notes * Rename enqueue_message to send_messgae (#7928) * [storage-blob, queue] Renames Logging to <ServiceName>AnalyticsLogging (#7921) * renames logging to ServiceAnalyticsLogging * changes param logging to analytics_logging * fixes tests due to param name change * [Blob][File][Encryption]Fix Download Encrypted Blob/File Bug (#7965) #7957 * Consolidate offset - length behavior in files (#7942) * Initital Commit * length changes * some test changes * some more changes * history.md * Update a couple of recrodings * comments address * ops * Stop modifying internal_response body (#7958) * Stop modifying internal body * Replicate for queues and files too * [Storage] Relocated SAS generation (#7955) * Expose account name on clients * Updated blob sas gen * Updated file sas gen * Update queue sas gen * Updated docstrings * Some pylint fixes * More pylint * More pylint * Reverted auth change * Added release notes * Fixed kwarg docstring * [Storage] Updated close handles (#7940) * Updated close handles * Pylint fixes * Fix tests * Review feedback * Missing recursive parameter * Added release notes * [Storage] Conditional etag parameters (#8047) * Updated clients * Process conditional headers * Updated tests * Fixed test * Updated error scenarios * Updated release notes * Revert skipped tests * [Storage] Download stream refactor (#7848) * Refactored download stream API * Unskip tests * Test warnings * Missing await * Fixed append tests * Fixed page tests * Pylint * Py2.7 iter support * Refactor downloaders * Added missing async module functions * Updated release notes * Keyword params * Documented more keyword options * Added module functions to exports * Fixed merge * Fix for Files download stream * Pylint fix * Updated File stream downloads * Fix tests * Updated error message * Added Files release notes * Pylint fix * Download stream error handling * Fixed some tests * Design Pipeline ownership (#7981) * Initital Commit * Pipeline Ownership * slight modifications * pipeline ownership for queues and files * fixes tests, adds docstrings to wrapper classes * pylint * Batching APIs Raise on Any Failure (#7963) * Raise on Single Failure * some changes * lint * some changes * oops * comments + lint * some optimization * history * comments * lint * recording * Plug HttpLoggingPolicy to Storage (#8081) * Plug HttpLoggingPolicy to Storage * Update dependencies * Skip depends job for Storage * Fix shared req * [storage] make storage files _internal (#7949) * blobs internal * files internal * queues internal * some fixes to docstrings * update history.md * fix async test import * some file mypy fixes * some queue mypy fixes * some blob mypy fixes * fix mistake in type annot * history edits * Merge fix * Revert "Merge fix" This reverts commit 748fbd6. * Better merge fix * fix some tests (#8100) * Fix tests (#8103) * minor fix special char * fix auth * comments * [Storage] Bumped version + Pipeline fix (#8089) * Bumped version * Docs tweak * Re-bumped version * Added 3.8 classifier * Added proxy policy to pipeline * Synced base clients * Added batch exception * Comment the echo check to workaround batch headers issue (#8118) * Fix 8091: expose generated enum to customers (#8117) * Fix #8091 * Add one in file

rakshith91 added 4 commits October 15, 2019 14:45

Initital Commit

79c3863

length changes

14f0c77

some test changes

17002e8

some more changes

37aa247

rakshith91 requested review from annatisch and zezha-msft as code owners October 16, 2019 17:56

Merge branch 'feature/storage-preview5' into offset-length

ef65959

rakshith91 requested review from kristapratico and xiafu-msft October 16, 2019 18:03

rakshith91 self-assigned this Oct 16, 2019

rakshith91 added blocking-release Blocks release Client This issue points to a problem in the data-plane of the library. P0 Storage Storage Service (Queues, Blobs, Files) labels Oct 16, 2019

history.md

5d4db54

kristapratico reviewed Oct 16, 2019

View reviewed changes

Update a couple of recrodings

2c17e8c

xiafu-msft reviewed Oct 17, 2019

View reviewed changes

comments address

54c9c70

annatisch reviewed Oct 17, 2019

View reviewed changes

ops

e82f978

annatisch approved these changes Oct 17, 2019

View reviewed changes

kristapratico approved these changes Oct 17, 2019

View reviewed changes

rakshith91 merged commit 95007d2 into Azure:feature/storage-preview5 Oct 17, 2019

rakshith91 deleted the offset-length branch October 17, 2019 18:22

Uh oh!

Conversation

rakshith91 commented Oct 16, 2019

Uh oh!

adxsdk6 commented Oct 16, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xiafu-msft Oct 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rakshith91 commented Oct 17, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

xiafu-msft Oct 17, 2019 •

edited

Loading